Fix silent migration error that caused failure to auto-create tasks from events#179
Fix silent migration error that caused failure to auto-create tasks from events#179srdco wants to merge 9 commits intojjdejong:masterfrom
Conversation
The event_after_insert trigger, which automatically creates tasks from rules when an event is created, was dropped by the utf8mb4_0900_ai_ci collation migration but not recreated on instances where the database user lacked the log_bin_trust_function_creators privilege at migration time. This left rules silently not firing on event creation, while the manual recreate_tasks stored procedure still worked. This migration drops and recreates the trigger with its original definition. It guards the operation with a privilege check: if log_bin_trust_function_creators cannot be set, it emits a clear warning and aborts rather than dropping the trigger and leaving the database broken. A similar warning is emitted if the CREATE TRIGGER itself fails after the drop. https://claude.ai/code/session_011sxfFyNJLM1JngEeGKFdoh
Claude/fix event rules tasks aj2aj -- fixes events not triggering due to collation change
DB::statement() uses PDO prepare/execute which fails with 'Wrong COM_STMT_PREPARE response size' for SET GLOBAL statements. Switch to DB::getPdo()->exec() which uses the exec protocol instead. Also make the privilege step best-effort rather than fatal: if binary logging is disabled the trigger can be created without the variable, so warn and continue instead of aborting on any failure. https://claude.ai/code/session_011sxfFyNJLM1JngEeGKFdoh
Fix COM_STMT_PREPARE error in recreate trigger migration
Previously the migration dropped event_after_insert unconditionally before attempting CREATE TRIGGER, leaving the database broken if the CREATE then failed due to lack of SUPER / log_bin_trust_function_creators. Now the DROP only happens when SET GLOBAL log_bin_trust_function_creators succeeded (i.e. we know we can recreate). Without privilege the migration still attempts CREATE (works when binary logging is off), but skips the DROP so any existing trigger is preserved on failure. Error messages now distinguish "dropped but failed" from "not dropped, just failed" and include the exact DBA command to unblock the migration. https://claude.ai/code/session_011sxfFyNJLM1JngEeGKFdoh
Only drop trigger after confirming privilege to recreate it
…lege requirement Previously the migration printed an error but still recorded itself as DONE, leaving the trigger absent with no indication to the user. Now it re-throws the exception so Laravel marks the migration as failed and it can be retried after fixing the privilege. Also documents in README that MySQL servers with binary logging require either SYSTEM_VARIABLES_ADMIN granted to the app user, or log_bin_trust_function_creators=1 in mysqld.cnf, before running migrations. https://claude.ai/code/session_011sxfFyNJLM1JngEeGKFdoh
Fail migration loudly on trigger creation error; document MySQL privi…
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a critical issue where a database migration could fail silently, leaving the application in a broken state without auto-created tasks. The changes are well-implemented, making the migration process more robust by conditionally dropping the trigger, using the correct PDO method for SET GLOBAL, and ensuring failures are loud and actionable. The documentation update is also a valuable addition. My review includes a couple of suggestions to further improve the maintainability of the new migration file.
| DB::unprepared(" | ||
| CREATE TRIGGER `event_after_insert` AFTER INSERT ON `event` FOR EACH ROW trig: BEGIN | ||
| DECLARE DueDate, BaseDate, m_expiry DATE DEFAULT NULL; | ||
| DECLARE tr_id, tr_days, tr_months, tr_years, m_pta, lnk_matter_id, CliAnnAgt, m_parent_id INT DEFAULT NULL; | ||
| DECLARE tr_task, m_type_code, tr_currency, m_country, m_origin CHAR(5) DEFAULT NULL; | ||
| DECLARE tr_detail, tr_responsible VARCHAR(160) DEFAULT NULL; | ||
| DECLARE Done, tr_clear_task, tr_delete_task, tr_end_of_month, tr_recurring, tr_use_priority, m_dead BOOLEAN DEFAULT 0; | ||
| DECLARE tr_cost, tr_fee DECIMAL(6,2) DEFAULT null; | ||
|
|
||
| DECLARE cur_rule CURSOR FOR | ||
| SELECT task_rules.id, task, clear_task, delete_task, detail, days, months, years, recurring, end_of_month, use_priority, cost, fee, currency, task_rules.responsible | ||
| FROM task_rules | ||
| JOIN event_name ON event_name.code = task_rules.task | ||
| JOIN matter ON matter.id = NEW.matter_id | ||
| WHERE task_rules.active = 1 | ||
| AND task_rules.for_category = matter.category_code | ||
| AND NEW.code = task_rules.trigger_event | ||
| AND (Now() < use_before OR use_before IS null) | ||
| AND (Now() > use_after OR use_after IS null) | ||
| AND IF (task_rules.for_country IS NOT NULL, | ||
| task_rules.for_country = matter.country, | ||
| concat(task_rules.task, task_rules.trigger_event) NOT IN (SELECT concat(tr.task, tr.trigger_event) FROM task_rules tr WHERE (tr.for_country, tr.for_category, tr.active) = (matter.country, matter.category_code, 1)) | ||
| ) | ||
| AND IF (task_rules.for_origin IS NOT NULL, | ||
| task_rules.for_origin = matter.origin, | ||
| concat(task_rules.task, task_rules.trigger_event) NOT IN (SELECT concat(tr.task, tr.trigger_event) FROM task_rules tr WHERE (tr.for_origin, tr.for_category, tr.active) = (matter.origin, matter.category_code, 1)) | ||
| ) | ||
| AND IF (task_rules.for_type IS NOT NULL, | ||
| task_rules.for_type = matter.type_code, | ||
| concat(task_rules.task, task_rules.trigger_event) NOT IN (SELECT concat(tr.task, tr.trigger_event) FROM task_rules tr WHERE (tr.for_type, tr.for_category, tr.active) = (matter.type_code, matter.category_code, 1)) | ||
| ) | ||
| AND NOT EXISTS (SELECT 1 FROM event WHERE event.matter_id = NEW.matter_id AND event.code = task_rules.abort_on) | ||
| AND IF (task_rules.condition_event IS NULL, true, EXISTS (SELECT 1 FROM event WHERE event.matter_id = NEW.matter_id AND event.code = task_rules.condition_event)); | ||
|
|
||
| DECLARE cur_linked CURSOR FOR | ||
| SELECT matter_id FROM event WHERE event.alt_matter_id = NEW.matter_id; | ||
|
|
||
| DECLARE CONTINUE HANDLER FOR NOT FOUND SET Done = 1; | ||
|
|
||
| SELECT type_code, dead, expire_date, term_adjust, country, origin, parent_id INTO m_type_code, m_dead, m_expiry, m_pta, m_country, m_origin, m_parent_id FROM matter WHERE matter.id = NEW.matter_id; | ||
| SELECT id INTO CliAnnAgt FROM actor WHERE display_name = 'CLIENT'; | ||
|
|
||
| -- Do not change anything in dead cases | ||
| IF (m_dead) THEN | ||
| LEAVE trig; | ||
| END IF; | ||
|
|
||
| OPEN cur_rule; | ||
| create_tasks: LOOP | ||
| SET BaseDate = NEW.event_date; | ||
| FETCH cur_rule INTO tr_id, tr_task, tr_clear_task, tr_delete_task, tr_detail, tr_days, tr_months, tr_years, tr_recurring, tr_end_of_month, tr_use_priority, tr_cost, tr_fee, tr_currency, tr_responsible; | ||
|
|
||
| IF Done THEN | ||
| LEAVE create_tasks; | ||
| END IF; | ||
|
|
||
| -- Skip renewal tasks if the client is the annuity agent | ||
| IF tr_task = 'REN' AND EXISTS (SELECT 1 FROM matter_actor_lnk lnk WHERE lnk.role = 'ANN' AND lnk.actor_id = CliAnnAgt AND lnk.matter_id = NEW.matter_id) THEN | ||
| ITERATE create_tasks; | ||
| END IF; | ||
|
|
||
| -- Skip recurring renewal tasks if country table has no correspondence | ||
| IF tr_task = 'REN' AND tr_recurring = 1 AND NOT EXISTS (SELECT 1 FROM country WHERE iso = m_country and renewal_start = NEW.code) THEN | ||
| ITERATE create_tasks; | ||
| END IF; | ||
|
|
||
| -- Use earliest priority date for task deadline calculation, if a PRI event exists | ||
| IF tr_use_priority THEN | ||
| SELECT CAST(IFNULL(min(event_date), NEW.event_date) AS DATE) INTO BaseDate FROM event_lnk_list WHERE code = 'PRI' AND matter_id = NEW.matter_id; | ||
| END IF; | ||
|
|
||
| -- Clear the task identified by the rule (do not create anything) | ||
| IF tr_clear_task THEN | ||
| UPDATE task JOIN event ON task.trigger_id = event.id | ||
| SET task.done_date = NEW.event_date | ||
| WHERE task.code = tr_task AND event.matter_id = NEW.matter_id AND task.done = 0; | ||
| ITERATE create_tasks; | ||
| END IF; | ||
|
|
||
| -- Delete the task identified by the rule (do not create anything) | ||
| IF tr_delete_task THEN | ||
| DELETE FROM task | ||
| WHERE task.code = tr_task AND task.trigger_id IN (SELECT event.id FROM event WHERE event.matter_id = NEW.matter_id); | ||
| ITERATE create_tasks; | ||
| END IF; | ||
|
|
||
| -- Calculate the deadline | ||
| SET DueDate = BaseDate + INTERVAL tr_days DAY + INTERVAL tr_months MONTH + INTERVAL tr_years YEAR; | ||
| IF tr_end_of_month THEN | ||
| SET DueDate = LAST_DAY(DueDate); | ||
| END IF; | ||
|
|
||
| -- Deadline for renewals that have become due in a parent case when filing a divisional (EP 4-months rule) | ||
| IF tr_task = 'REN' AND m_parent_id IS NOT NULL AND DueDate < NEW.event_date THEN | ||
| SET DueDate = NEW.event_date + INTERVAL 4 MONTH; | ||
| END IF; | ||
|
|
||
| -- Don't create tasks having a deadline in the past, except for expiry and renewal | ||
| -- Create renewals up to 6 months in the past in general, create renewals up to 19 months in the past for PCT national phases (captures direct PCT filing situations) | ||
| IF (DueDate < Now() AND tr_task NOT IN ('EXP', 'REN')) | ||
| OR (DueDate < (Now() - INTERVAL 6 MONTH) AND tr_task = 'REN' AND m_origin != 'WO') | ||
| OR (DueDate < (Now() - INTERVAL 19 MONTH) AND tr_task = 'REN' AND m_origin = 'WO') | ||
| THEN | ||
| ITERATE create_tasks; | ||
| END IF; | ||
|
|
||
| IF tr_task = 'EXP' THEN | ||
| -- Handle the expiry task (no task created) | ||
| UPDATE matter SET expire_date = DueDate + INTERVAL m_pta DAY WHERE matter.id = NEW.matter_id; | ||
| ELSEIF tr_recurring = 0 THEN | ||
| -- Insert the new task if it is not a recurring one | ||
| INSERT INTO task (trigger_id, code, due_date, detail, rule_used, cost, fee, currency, assigned_to, creator, created_at, updated_at) | ||
| VALUES (NEW.id, tr_task, DueDate, tr_detail, tr_id, tr_cost, tr_fee, tr_currency, tr_responsible, NEW.creator, Now(), Now()); | ||
| ELSEIF tr_task = 'REN' THEN | ||
| -- Recurring renewal is the only possibility here, normally | ||
| CALL insert_recurring_renewals(NEW.id, tr_id, BaseDate, tr_responsible, NEW.creator); | ||
| END IF; | ||
| END LOOP create_tasks; | ||
| CLOSE cur_rule; | ||
| SET Done = 0; | ||
|
|
||
| -- If the event is a filing, update the tasks of the matters linked by event (typically the tasks based on the priority date) | ||
| IF NEW.code = 'FIL' THEN | ||
| OPEN cur_linked; | ||
| recalc_linked: LOOP | ||
| FETCH cur_linked INTO lnk_matter_id; | ||
| IF Done THEN | ||
| LEAVE recalc_linked; | ||
| END IF; | ||
| CALL recalculate_tasks(lnk_matter_id, 'FIL', NEW.creator); | ||
| END LOOP recalc_linked; | ||
| CLOSE cur_linked; | ||
| END IF; | ||
|
|
||
| -- Recalculate tasks based on the priority date upon inserting a new priority claim | ||
| IF NEW.code = 'PRI' THEN | ||
| CALL recalculate_tasks(NEW.matter_id, 'FIL', NEW.creator); | ||
| END IF; | ||
|
|
||
| -- Set matter to dead upon inserting a killer event | ||
| SELECT killer INTO m_dead FROM event_name WHERE NEW.code = event_name.code; | ||
| IF m_dead THEN | ||
| UPDATE matter SET dead = 1 WHERE matter.id = NEW.matter_id; | ||
| END IF; | ||
| END trig | ||
| "); |
There was a problem hiding this comment.
The CREATE TRIGGER statement is very large and embedded as a string in the PHP file. This can make it difficult to read, maintain, and validate with SQL tools. Consider moving this complex SQL logic into a separate .sql file and loading it within the migration. This improves code separation and maintainability.
For example:
DB::unprepared(file_get_contents(__DIR__.'/../sql/recreate_event_after_insert_trigger.sql'));| echo "[ERROR] The event_after_insert trigger was dropped but could not be recreated: " . $e->getMessage() . "\n" | ||
| . " Rules will NOT fire automatically when events are created until this is fixed.\n" | ||
| . " To fix manually, ask a DBA to run the CREATE TRIGGER statement\n" | ||
| . " found in database/schema/mysql-schema.sql (around line 282).\n"; |
There was a problem hiding this comment.
The error message contains a hardcoded line number reference: (around line 282). This is brittle and can easily become outdated if database/schema/mysql-schema.sql is modified. It would be more robust to refer to the trigger definition by name instead of a specific line number.
. " found in the `event_after_insert` trigger definition within database/schema/mysql-schema.sql.\n";2 participants
Adds a migration to recreate the trigger (2026_03_23_000000_recreate_event_after_insert_trigger.php)
Drops and recreates event_after_insert with its original full definition. This is a one-time repair migration for affected installations.
Fixes a COM_STMT_PREPARE protocol error
DB::statement() uses PDO's prepare/execute protocol, which MySQL rejects for SET GLOBAL statements. Switched to DB::getPdo()->exec() which uses the correct direct-execution protocol.
Makes the DROP conditional on having the privilege to recreate
The original migration dropped the trigger unconditionally before attempting CREATE TRIGGER, which could leave the database in a broken state if CREATE then failed. Now the DROP only happens after successfully setting log_bin_trust_function_creators = 1 — confirming we can recreate. If the privilege is absent, the existing trigger is left untouched and creation is still attempted (works when binary logging is off).
Makes migration failure loud and actionable
Previously the migration caught the exception, printed a warning, and still recorded itself as DONE — leaving the trigger absent with no visible indication. Now it re-throws the exception so Laravel marks the migration as failed, keeps it pending, and the operator can retry after fixing the privilege. Error messages distinguish between "trigger was dropped but failed to recreate" and "trigger was not dropped, creation failed" and include the exact DBA command to unblock.
Documents the MySQL privilege requirement in the README
New installs on servers with binary logging enabled will see a clear explanation of the two options: grant SYSTEM_VARIABLES_ADMIN to the app user (MySQL 8.0+), or set log_bin_trust_function_creators = 1 in mysqld.cnf.